src/blueprints/bookfusion_bp.py (1)

80-93: Consider using lazy logging format instead of f-strings.

F-strings in logging calls (e.g., logger.error(f"...")) evaluate even when the log level is disabled. Minor performance impact, but standard practice is logger.error("message: %s", e).

♻️ Optional: Use lazy logging format

     except Exception as e:
-        logger.error(f"Failed to read ebook file: {e}")
+        logger.error("Failed to read ebook file: %s", e)
         return jsonify({"error": "Failed to read ebook file"}), 500

     title = book.title or ""
     authors = book.author or ""

-    logger.info(f"BookFusion upload request: title='{title}', authors='{authors}', filename='{ebook_filename}'")
+    logger.info("BookFusion upload request: title='%s', authors='%s', filename='%s'", title, authors, ebook_filename)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/blueprints/bookfusion_bp.py` around lines 80 - 93, Replace f-strings in
logging calls with lazy logging format: in the except block for reading the
ebook file, change logger.error(f"Failed to read ebook file: {e}") to use
parameterized logging with the exception object (e.g., logger.error("Failed to
read ebook file: %s", e)); likewise change the logger.info call that logs
title/authors/filename to logger.info("BookFusion upload request: title=%s,
authors=%s, filename=%s", title, authors, ebook_filename). Keep the existing
variables (file_bytes, title, authors, ebook_filename) and the
bf_client.upload_book call unchanged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/utils/runtime_config.py (1)

6-26: Add type annotations to match the documented signatures.

The AI summary advertises typed signatures (e.g., get_str(key, default="") -> str), but the implementation has none. Adding them improves IDE/type-checker support (Pyright is in your toolchain per the PR).

♻️ Proposed annotations

-def get_str(key, default=""):
+def get_str(key: str, default: str = "") -> str:
     return os.environ.get(key, default)


-def get_bool(key, default=False):
+def get_bool(key: str, default: bool = False) -> bool:
     raw_default = "true" if default else "false"
     return os.environ.get(key, raw_default).strip().lower() in ("true", "1", "yes", "on")


-def get_int(key, default):
+def get_int(key: str, default: int) -> int:
     try:
         return int(os.environ.get(key, str(default)))
     except (TypeError, ValueError):
         return int(default)


-def get_float(key, default):
+def get_float(key: str, default: float) -> float:
     try:
         return float(os.environ.get(key, str(default)))
     except (TypeError, ValueError):
         return float(default)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/runtime_config.py` around lines 6 - 26, Annotate the runtime config
helpers in src/utils/runtime_config.py: give each function a typed signature
(key: str) and explicit parameter and return types — get_str(key: str, default:
str = "") -> str; get_bool(key: str, default: bool = False) -> bool;
get_int(key: str, default: int) -> int; get_float(key: str, default: float) ->
float — so IDEs and type-checkers (Pyright) understand the expected types;
adjust any imports if needed for typing but no behavior changes to the bodies of
get_str, get_bool, get_int, or get_float.

src/api/hardcover_client.py (1)

92-112: Refactor LGTM — one tiny inconsistency.

Delegating retry/backoff to the base class is a clean improvement, and the _mark_retry callback correctly updates _last_request_time after the backoff sleep. Minor nit: line 111 hardcodes "after 3 retries" while max_retries=3 is also inline at line 103 — if either ever changes, they'll drift. Use the same constant:

-        max_retries=3,
+        max_retries=(_max_retries := 3),
...
-                logger.error("Hardcover rate limit persisted after 3 retries, giving up")
+                logger.error("Hardcover rate limit persisted after %d retries, giving up", _max_retries)

(Or just lift MAX_RETRIES = 3 to a class constant.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/hardcover_client.py` around lines 92 - 112, The log message hardcodes
"after 3 retries" which can drift from the max_retries passed to
post_json_with_retries; change the code to refer to a single source of truth
(e.g. introduce a class constant MAX_RETRIES = 3 or reuse the local max_retries
variable) and use that identifier in the retry call and in the logger message
(refer to the post_json_with_retries call, the max_retries argument, the
_mark_retry callback, and the r.status_code == 429 logger.error) so the message
and the retry configuration always stay in sync.

tests/test_cache_cleanup_service.py (1)

7-28: Consider adding edge-case coverage.

Happy path is covered well. A few low-cost additions worth tagging on:

• cleanup() is a no-op when epub_cache_dir doesn't exist (early return at line 14).
• suggestion.matches is None / non-list (which today would silently abort the whole pass — see comment in cache_cleanup_service.py).
• A file whose unlink() raises — verifies the per-file except keeps the loop running.

Not blocking, but these are the paths most likely to regress.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_cache_cleanup_service.py` around lines 7 - 28, Add unit tests to
cover the noted edge cases for CacheCleanupService.cleanup(): test that
cleanup() returns early (no-op) when epub_cache_dir does not exist; test that
suggestions with suggestion.matches set to None or a non-list do not raise and
are skipped (locate usage around suggestion.matches in
CacheCleanupService.cleanup); and test that when Path.unlink() raises for a
specific cached file the exception is caught and the loop continues (simulate by
mocking Path.unlink or creating a file and patching unlink to raise while
asserting other files are still removed). Ensure tests instantiate
CacheCleanupService with a Mock db (get_all_books /
get_all_actionable_suggestions) and verify expected files remain or are removed
after service.cleanup().

pyrightconfig.json (1)

7-16: Heads up: this config silences a lot of pyright's actual value.

Disabling reportMissingImports, reportCallIssue, reportArgumentType, reportAssignmentType, and reportOptionalMemberAccess simultaneously means pyright will essentially only flag syntax/parse errors. If the goal is to silence noise from optional deps (e.g., alembic), consider narrowing via per-file # pyright: ignore or defineConstant/stub paths rather than globally muting whole categories. At minimum, reportCallIssue and reportAssignmentType catch real bugs and are worth keeping at "warning".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pyrightconfig.json` around lines 7 - 16, The pyright configuration currently
mutes many important diagnostics (reportMissingImports,
reportMissingModuleSource, reportAttributeAccessIssue, reportArgumentType,
reportOptionalMemberAccess, reportOptionalOperand, reportOptionalSubscript,
reportAssignmentType, reportCallIssue, reportIncompatibleMethodOverride), which
hides real bugs; change at least reportCallIssue and reportAssignmentType back
to "warning" (instead of "none") in pyrightconfig.json, and avoid globally
disabling reportMissingImports/reportArgumentType — instead add targeted
per-file directives (# pyright: ignore) or use defineConstant/stub paths for
optional deps like alembic to reduce noise while preserving useful checks.
Ensure the unique keys reportCallIssue and reportAssignmentType are updated and
consider selectively re-enabling reportMissingImports or handling imports via
type stubs rather than keeping them set to "none".

src/api/http_client_base.py (2)

1-1: Pyright suppression at file scope is broad.

# pyright: reportMissingImports=false, reportMissingModuleSource=false here is redundant given pyrightconfig.json already disables both globally. Either drop these per-file pragmas (DRY) or, conversely, drop them from the global config and keep them local where actually needed. Mixing both makes future tightening harder.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/http_client_base.py` at line 1, The file-scope Pyright pragma "#
pyright: reportMissingImports=false, reportMissingModuleSource=false" is
redundant with the global pyrightconfig.json; remove this per-file pragma to
avoid DRY/redundant suppressions (or alternatively remove those global disables
and keep the per-file pragma if the intent is local-only), so update the file by
deleting the "# pyright: reportMissingImports=false,
reportMissingModuleSource=false" line (or coordinate with the team to remove the
same rules from pyrightconfig.json if you prefer local pragmas).

---

30-50: Network exceptions bypass the retry loop.

request_func(...) on lines 30 and 47 isn't wrapped, so requests.exceptions.ConnectionError, Timeout, etc. propagate immediately — no retry, no backoff. For the current Hardcover-only caller (which only retries 429), that's arguably intentional. But if this base class is going to be reused (the name suggests it will), transient network failures are usually the first thing you'd want to retry. Consider treating exceptions raised by request_func as a retryable signal too, e.g.:

🔧 Sketch

-        retry_statuses = set(retry_statuses or [])
-        request_func = request_func or requests.post
-        response = request_func(url, json=json_body, headers=headers, timeout=timeout)
-        attempt = 0
-        backoff = backoff_seconds
-
-        while response.status_code in retry_statuses and attempt < max_retries:
+        retry_statuses = set(retry_statuses or [])
+        retry_exceptions = (requests.exceptions.ConnectionError, requests.exceptions.Timeout)
+        request_func = request_func or requests.post
+        attempt = 0
+        backoff = backoff_seconds
+
+        def _do_request():
+            return request_func(url, json=json_body, headers=headers, timeout=timeout)
+
+        try:
+            response = _do_request()
+        except retry_exceptions:
+            if max_retries <= 0:
+                raise
+            response = None
+
+        while attempt < max_retries and (response is None or response.status_code in retry_statuses):
             ...

Also worth considering jitter on backoff *= 2 if multiple workers ever share this client.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/http_client_base.py` around lines 30 - 50, Wrap calls to request_func
in a try/except that catches requests.exceptions.RequestException (or the
broader RequestException family) and treat exceptions the same as retryable
responses: increment attempt, call on_retry with the exception (e.g.,
on_retry(attempt, exception)), sleep using backoff (adding optional jitter),
multiply backoff (backoff *= 2) and continue retrying until max_retries; apply
this handling for both the initial request and subsequent retries in the loop
where retry_statuses, backoff_seconds, max_retries, request_func, and on_retry
are used so transient network errors are retried consistently with status-code
retries.

src/db/base_repository.py (1)

40-47: Optional: rename first flag to avoid shadowing query API name.

first collides conceptually with query.first() in the body and forces a positional bool flag at call sites. A keyword-only param like one=False (matching the AI summary) reads cleaner; or split into _query_one_and_expunge / _query_all_and_expunge to drop the flag entirely. Non-blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/db/base_repository.py` around lines 40 - 47, Rename the boolean flag on
_query_and_expunge to avoid shadowing query.first() and to make call sites
clearer: change the signature of _query_and_expunge (and its callers) to use a
keyword-only parameter like one=False (or split into two methods
_query_one_and_expunge and _query_all_and_expunge) so the intent isn’t confused
with query.first(); inside the function keep the same logic (use query.first()
and session.expunge(obj) for the single-case, and query.all() with
_expunge_items for the multi-case) but update all invocations to pass the new
keyword or call the new split methods.

src/app_setup.py (1)

5-5: Drop unused Path import (Ruff F401).

♻️ Suggested change

-from pathlib import Path
-
 from dependency_injector import providers

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/app_setup.py` at line 5, Remove the unused import Path from
src/app_setup.py by deleting the "Path" symbol from the import statement (the
line "from pathlib import Path"); ensure no other code references Path in
functions or classes in this module (if any references exist, either restore
usage or import the needed symbol where it's actually used).

templates/reading_detail.html (1)

549-551: Dead data-book-id attribute on bulk-import button.

The handler at line 1223-1255 reads from _rdAbsId, not dataset.bookId, so data-book-id="{{ book.id }}" is dead. Either drop the attribute or align the handler to use it. Cosmetic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@templates/reading_detail.html` around lines 549 - 551, The bulk import button
with id "bulk-import-highlights" currently includes a dead data-book-id="{{
book.id }}" attribute because the click handler uses the _rdAbsId variable (not
dataset.bookId); either remove the unused attribute from the <button> or change
the handler to read event.currentTarget.dataset.bookId and use that value
instead of _rdAbsId (update the handler referenced by its id
"bulk-import-highlights" and any functions that rely on _rdAbsId to accept the
bookId from dataset).

src/services/sync_manager_startup.py (1)

54-61: Optional: drop the try/except around hasattr.

hasattr doesn't raise (since Python 3.2 it swallows exceptions internally), so wrapping it in try/except is unreachable defensive code. Either drop the wrapper or call the methods to actually exercise them.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/sync_manager_startup.py` around lines 54 - 61, Remove the
unreachable try/except wrapper around the hasattr checks: inside the
is_configured() branch, directly test hasattr(self.abs_client,
"get_ebook_files") and hasattr(self.abs_client, "search_ebooks") and log via
logger.info/logger.warning accordingly; delete the surrounding try and the
except that calls sanitize_exception(exc) so the code is simpler and not
guarding against an impossible hasattr exception on self.abs_client.

src/blueprints/reading_bp.py (2)

691-691: Inconsistent error response style.

This branch still uses raw jsonify({"success": False, "error": ...}), 400 while the rest of the function (and file) was migrated to json_error(...). Tighten consistency:

♻️ Suggested change

-            return jsonify({"success": False, "error": "Invalid date format (expected YYYY-MM-DD)"}), 400
+            return json_error("Invalid date format (expected YYYY-MM-DD)", 400)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/blueprints/reading_bp.py` at line 691, The branch currently returns
jsonify({"success": False, "error": "Invalid date format (expected
YYYY-MM-DD)"}), 400 which is inconsistent with the file's standard error helper;
replace that raw jsonify return with a call to the common json_error helper
(e.g. json_error("Invalid date format (expected YYYY-MM-DD)", 400)) so the
function uses json_error everywhere instead of jsonify; update the return site
where the current jsonify call appears and remove the old jsonify usage.

---

3-26: Sort imports (Ruff I001).

Static analysis flags the import block as unsorted; from src.utils.http import json_error was inserted out of order. Run ruff check --fix or move it next to the other src.utils.* imports.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/blueprints/reading_bp.py` around lines 3 - 26, Import ordering in
src.blueprints.reading_bp.py is unsorted: move the "from src.utils.http import
json_error" import into the block of other src.utils imports so the src.utils.*
imports are contiguous and alphabetically ordered (e.g., ensure json_error
appears alongside resolve_book_covers and render_markdown_html imports or
grouped with other src.utils imports); run ruff check --fix or manually reorder
the import block to match Ruff I001 expectations while preserving existing
import names like json_error, resolve_book_covers, and render_markdown_html.

src/blueprints/bookfusion_bp.py (1)

178-198: linked_count is a constant — and sync_all_highlights ignores abs_id.

Two nits in /api/bookfusion/sync-book:

1. The loop just increments linked_count per bf_id, so it's always len(bf_book_ids). Either count actual link-result rows (e.g., return value of link_bookfusion_highlights_by_book_id) or drop it.
2. bf_client.sync_all_highlights(db_service) syncs the entire BookFusion library on every per-book request. For users with large libraries and the "Re-sync All" UI calling this for many books, this is O(N²) traffic. Consider a per-book sync (or pass bf_book_ids to scope the sync) if BookFusion's API supports it.

♻️ Suggested cleanup for `#1`

-        linked_count = 0
-        for bf_id in bf_book_ids:
-            db_service.link_bookfusion_highlights_by_book_id(bf_id, book.id)
-            linked_count += 1
+        for bf_id in bf_book_ids:
+            db_service.link_bookfusion_highlights_by_book_id(bf_id, book.id)
@@
-                "linked_books": linked_count,
+                "linked_books": len(bf_book_ids),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/blueprints/bookfusion_bp.py` around lines 178 - 198, The loop increments
linked_count blindly and bf_client.sync_all_highlights(db_service) syncs the
whole library on a per-book endpoint; fix by (A) making
link_bookfusion_highlights_by_book_id return the number of rows linked and use
that return value to accumulate linked_count (replace the manual ++ with adding
the actual result from db_service.link_bookfusion_highlights_by_book_id), or
simply drop linked_count if you don't need it, and (B) avoid calling
bf_client.sync_all_highlights for a single-book request — either add/use a
scoped sync method (e.g., bf_client.sync_highlights_for_book(bookfusion_id,
db_service) or accept bf_book_ids in sync_all_highlights) and call that with
each bf_id (or call one scoped sync for the single book before linking) to
prevent O(N²) full-library syncs.

src/services/kosync_progress_service.py (1)

17-21: Reaching into service._db / _container / _manager couples this class to private internals.

If KosyncService ever renames or refactors those attributes, this service silently breaks at construction (or worse, at first call). Since KosyncProgressService already needs all three, prefer passing them explicitly from the composition root, or expose public accessors on KosyncService.

♻️ Sketch

-    def __init__(self, service):
-        self.service = service
-        self.db = service._db
-        self.container = service._container
-        self.manager = service._manager
+    def __init__(self, service, *, db, container, manager):
+        self.service = service
+        self.db = db
+        self.container = container
+        self.manager = manager

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/kosync_progress_service.py` around lines 17 - 21,
KosyncProgressService currently reaches into KosyncService private internals in
__init__ by accessing service._db, service._container, and service._manager;
update the constructor to avoid coupling by either (A) changing
KosyncProgressService.__init__ to accept the dependencies explicitly (db,
container, manager) and wire them from the composition root where KosyncService
is constructed, or (B) add and use public accessors on KosyncService (e.g.,
get_db(), get_container(), get_manager()) and replace uses of
service._db/_container/_manager with those methods; modify all call sites that
construct KosyncProgressService to pass the new arguments or to call the new
accessors accordingly so no private attributes are accessed directly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes